Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cut option maria_role as it has nothing to do with roles #579

Conversation

laurent-indermuehle
Copy link
Collaborator

@laurent-indermuehle laurent-indermuehle commented Oct 11, 2023

SUMMARY

This option was introduced in #189. To my knowledge, there is no difference between MySQL and MariaDB regarding roles or when you call a user by its name alone. Same for roles.

For instance:

create user foo;
select user, host from mysql.user;
-- foo, %
show grants for foo;
-- GRANT USAGE ON *.* TO foo@%;

create role role_foo;
select user, host from mysql.user;
-- foo, %
-- role_foo, %
show grants for role_foo;

So both in MySQL and MariaDB you can omit the host.

The mysql_user plugin defaults host to localhost: host=dict(type='str', default='localhost')
So all functions will always receive a host. Maybe the option was used to bypass the default, 'localhost' and search for '%' instead of localhost ? This works because when you don't specify a host, MySQL and MariaDB will assume host is '%'.
But then, I don't get why the search for '%' is made by an option called maria_role.

I removed this option and if all tests path, I propose the get rid of it.

Sometime we must know if we are in MySQL or MariaDB, like when parsing the return of SHOW GRANTS FOR. For those cases I added a new function to the module_utils mysql.py called get_server_type().

ISSUE TYPE
  • Code clarity Pull Request
COMPONENT NAME
  • mysql_user
  • mysql_role
  • module_utils: mysql

This was introduced in ansible-collections#189. To my knowledge, there is no difference
between MySQL and MariaDB regarding roles or when you call a user by
its name alone. Both works if the host it '%'. Same for roles.
@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (033b4c7) 76.35% compared to head (3da5b34) 71.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #579      +/-   ##
==========================================
- Coverage   76.35%   71.68%   -4.68%     
==========================================
  Files          28       15      -13     
  Lines        2394     2232     -162     
  Branches      584      561      -23     
==========================================
- Hits         1828     1600     -228     
- Misses        390      447      +57     
- Partials      176      185       +9     
Flag Coverage Δ
integration 71.55% <81.48%> (-2.29%) ⬇️
sanity ?
units ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
plugins/module_utils/mysql.py 63.41% <100.00%> (-0.76%) ⬇️
plugins/modules/mysql_role.py 69.23% <ø> (-12.18%) ⬇️
plugins/module_utils/user.py 80.25% <79.16%> (-5.04%) ⬇️

... and 20 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@laurent-indermuehle
Copy link
Collaborator Author

I was wrong. MySQL and MariaDB do differ:

-- MYSQL
+------------------+-----------+
| user             | host      |
+------------------+-----------+
| role_foo         | %         |
+------------------+-----------+

-- MariaDB
+-------------+-----------+
| User        | Host      |
+-------------+-----------+
| role_foo    |           |
+-------------+-----------+

So I close this PR. Sorry for the noise.

@laurent-indermuehle laurent-indermuehle deleted the lie_cut_option_maria_role branch October 11, 2023 14:37
@Andersson007
Copy link
Collaborator

@laurent-indermuehle no worries:)

@Andersson007
Copy link
Collaborator

BTW maybe we could add this table above to the code as a comment, so that it's seen why it works that way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants